Skip to content
This repository was archived by the owner on Oct 25, 2021. It is now read-only.

Filipchik_Kirill#30

Open
filipchikkirill wants to merge 84 commits intomasterfrom
feature/KirillFilipchik
Open

Filipchik_Kirill#30
filipchikkirill wants to merge 84 commits intomasterfrom
feature/KirillFilipchik

Conversation

@filipchikkirill
Copy link
Collaborator

No description provided.

@heptaber heptaber added readyForReview Sign for Artem to take a look and removed readyForReview Sign for Artem to take a look labels Jul 10, 2021
@NikolaevArtem NikolaevArtem added the bug Code fix needed label Jul 11, 2021
@NikolaevArtem NikolaevArtem changed the title FilipchikKirill_homework Filipchik_Kirill Jul 11, 2021
moved the creation of result String -> new method createStringResult
removed check for missing args
…chik

# Conflicts:
#	README.md
#	src/main/java/homework_1/Main.java
@filipchikkirill filipchikkirill added the readyForReview Sign for Artem to take a look label Jul 12, 2021
add color -> instead -> System.err
the last commit deleted the main, returned it.
@NikolaevArtem NikolaevArtem added HW_1 Good to go style Style fix needed and removed bug Code fix needed readyForReview Sign for Artem to take a look labels Jul 13, 2021

public class Wrapper <T> implements Problem<T> {
private final Problem<T> innerProblem;
private final List<String> event = new ArrayList<>();
Copy link
Owner

@NikolaevArtem NikolaevArtem Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good implementation for generating events!


public static void main(String[] args) {
Kitten kitten = new Kitten("Murzik", 1);
KittenToCatFunction function = (k) -> new Cat(k.getName(), k.getAge() + 4);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: you could add some more complicated logic to lambda, as example - converting one type to another (like, favourite toy to favourite color or smth like this)


import course_project.sea_battle.controller.impl.GameControllerImpl;

public class SeaBattleApp {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: The field is quite narrow, i think it would look better with more space between cells by width
image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The game seems not working. I see you implemented PvP, but I play only for 1 Player. I don't see field for 2nd, don't see his name anywhere. Don't know how to shoot for 2nd.
And please introduce automatic ship placement, testing will be much easier

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good! Nice architecture, SOLID is followed. Code is pretty readable. Good Java Core and Java libraries usage.

To make better: interface could be refactored. Also check correct working of the game.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also nice approach to testing, test cases are readable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, automatic ship placement! I found some minor issues with the interface:

  1. When miss, there's no message to press enter to finish the turn. Actually, I didn't see this message anywhere... Confusing
  2. In case i hit a ship, i need to press enter 2 times to be able to shoot again
  3. No error message in case I already shot this cell
    image
  4. No message in case of destroyed ship
    image
  5. And please make field representation wider :)

The app now works correctly, I didn't find any bugs. And from the code perspective, the app looks great!

To make better: improve interface:)
The homework is approved

@NikolaevArtem NikolaevArtem added HW_5 bug Code fix needed labels Sep 21, 2021
Copy link
Owner

@NikolaevArtem NikolaevArtem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The code needs small fixes, but except that - good job! The architecture is nice, and I see you exercised with inheritance/generics, which is cool. But remember that it's nice for learning purposes, but production code, at best, should be readable and simple first. Usually, when there's a choice between SOLID or readability/simplicity, I choose second. IMHO

@filipchikkirill filipchikkirill added the readyForReview Sign for Artem to take a look label Sep 21, 2021
@NikolaevArtem NikolaevArtem added Course Project HW_3 and removed bug Code fix needed labels Sep 23, 2021
Comment on lines +18 to +37
try {
run1();
} catch (IOException e) {
e.printStackTrace();
}
try {
run2();
} catch (IOException e) {
e.printStackTrace();
}
try {
run3();
} catch (IOException e) {
e.printStackTrace();
}
try {
run4();
} catch (IOException e) {
e.printStackTrace();
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: these try-catch now are either not needed or not in right place...

@NikolaevArtem NikolaevArtem added HW_4 HW_6 and removed readyForReview Sign for Artem to take a look labels Sep 23, 2021
Copy link
Owner

@NikolaevArtem NikolaevArtem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants